Skip to content

XHTTP: only force trailing path slash when session/seq use path placement#6307

Open
dudkin-2005 wants to merge 1 commit into
XTLS:mainfrom
dudkin-2005:xhttp-conditional-trailing-slash
Open

XHTTP: only force trailing path slash when session/seq use path placement#6307
dudkin-2005 wants to merge 1 commit into
XTLS:mainfrom
dudkin-2005:xhttp-conditional-trailing-slash

Conversation

@dudkin-2005

@dudkin-2005 dudkin-2005 commented Jun 10, 2026

Copy link
Copy Markdown

GetNormalizedPath always appends a trailing slash to the configured path.
That slash is only meaningful when sessionID and/or seq are carried in the
path: it separates the configured base path from the appended segments and
lets the server slice them back off via req.URL.Path[len(path):].

When both sessionID and seq are placed in query/cookie/header, the path
carries no appended segments, so the forced trailing slash is unnecessary.
It also makes URLs look unnatural — a directory-style trailing slash right
before the query string, or a file path that gets a slash appended after its
extension.

This change makes the trailing slash conditional on path placement being
used for sessionID or seq. The default placement is path, so any config
that does not move both fields off the path keeps the exact previous
behaviour; only configs that move both fields off the path are affected.

Examples

Both meta fields in query

"xhttpSettings": {
  "mode": "packet-up",
  "path": "/stream",
  "sessionIDPlacement": "query",
  "seqPlacement": "query"
}

Before:

GET  /stream/?x_session=<id>
POST /stream/?x_seq=0&x_session=<id>

After:

GET  /stream?x_session=<id>
POST /stream?x_seq=0&x_session=<id>

File-like path — "path": "/stream/filename.extension", both in query:
Before → /stream/filename.extension/?x_session=<id>
After → /stream/filename.extension?x_session=<id>

Unchanged: default (path) placement — "path": "/abc":

GET  /abc/<sessionID>
POST /abc/<sessionID>/<seq>

sessionID and seq are in the path, so the trailing slash is still added — identical to the current behaviour.

Unchanged: root path — "path": "/" stays /?... in all cases.

@dudkin-2005 dudkin-2005 force-pushed the xhttp-conditional-trailing-slash branch from 8a45b8e to da21a8f Compare June 10, 2026 17:37
@dudkin-2005 dudkin-2005 reopened this Jun 10, 2026
@artur002

Copy link
Copy Markdown

By the way, you can check the patch and include this code snippet in the branch. You do realize this is an inherent shortcoming, right? Xray has always been highly flexible when it comes to traffic configuration. This looks more like an oversight.

@ghost

ghost commented Jun 17, 2026

Copy link
Copy Markdown

@RPRX merge

@RPRX

RPRX commented Jun 17, 2026

Copy link
Copy Markdown
Member

我得评估下新旧版本兼容性问题,盲猜是 breaking@dudkin-2005 你测过吗

@dudkin-2005

dudkin-2005 commented Jun 17, 2026

Copy link
Copy Markdown
Author

我得评估下新旧版本兼容性问题,盲猜是 breaking@dudkin-2005 你测过吗

Yes, I tested this; an xray update on the server is required because it can't validate the path.

[Info] transport/internet/splithttp: failed to validate path, request:/stream/filename.extension, config:/stream/filename.extension/

If the client uses a version of xray without this commit, and the server does have this commit, then this will work, because the serveHttp function in hub.go checks whether the path of the incoming HTTP request starts with a certain prefix:

	if !strings.HasPrefix(request.URL.Path, h.path) {
		errors.LogInfo(context.Background(), "failed to validate path, request:", request.URL.Path, ", config:", h.path)
		writer.WriteHeader(http.StatusNotFound)
		return
	}

@ghost

ghost commented Jun 18, 2026

Copy link
Copy Markdown

我得评估下新旧版本兼容性问题,盲猜是 breaking,@dudkin-2005 你测过吗

you've already committed breaking change with sessionTables, it won't change much, people who need backward compatibility can add / at the end of their path, additionally you can create a sub option in config in extra to disable slash behavior for query mode if you really need that, but i dont see any reasons to, query and path are split into two different independent chunks even by nginx

@PandaWorker

PandaWorker commented Jun 29, 2026

Copy link
Copy Markdown

In xhttp, path assembly and parsing was initially poorly designed for flexibility, I would like to hear opinions on why template uris were not used, such as in masque, but with the flexibility of keys.

path-template: "/api/v1/users/{session}/posts/{seq}/image.png"
seq-key: seq
seq-placement: path-template
session-key: session
session-placement: path-template
path-template: "/api/v1/users/{session}/posts{?offset}"
seq-key: offset
seq-placement: path-template
session-key: session
session-placement: path-template

@nehamoff

Copy link
Copy Markdown

@RPRX merge, on my configs, they added a block on paths with / at the end, and most people encountered this problem. This is a common issue on many CDN services.

@artur002

artur002 commented Jun 29, 2026

Copy link
Copy Markdown

@RPRX I don't quite get why still force a trailing slash at the end of the path string with no way to disable it.

Right now, using a clean endpoint like /name.file transforms the request into /name.file/?... The requests never even reach the server.

If the main reason for not merging a fix is the fear of breaking backward compatibility — why not introduce an exclamation mark "!" at the end of the path as an opt-in flag? If present, the core strips it and leaves the path completely clean, without appending any slash. Old configs won't be affected at all.

Example: "path": "/name.file!" -> emits a clean /name.file over the wire.

Implementation snippet:

if path[len(path)-1] != '/' {
    if path[len(path)-1] == '!' {
        path = path[:len(path)-1]
    } else {
        path = path + "/"
    }

@vadimsky-ctrl

Copy link
Copy Markdown

Adding the concrete impact, since it's a bit stronger than "looks unnatural": on some CDNs the forced trailing slash is the difference between the tunnel working and a hard 403 on every request — minimal repro in #6410. A path ending in / is treated as dynamic and its query string is scanned by an anti-cache-poisoning WAF (which matches the sessionId token), while a path ending in <name>.<ext> is treated as a static asset and skipped. Removing the slash gives 25/25 204 end-to-end vs. a 100% 403 rate with the same config otherwise.

On the backward-compat concern: the only breaking direction is new client → old server — the old server normalizes its own path to …/, so HasPrefix(request.Path, config.Path) then rejects the slash-less request (matches @dudkin-2005's failed to validate path log). An opt-in form sidesteps that completely, since you'd only enable it on a matched client+server pair while every existing config stays byte-identical — e.g. @artur002's ! suffix ("path": "/name.file!" → clean /name.file) or a small extra flag. Given how common this CDN block is, an opt-in looks like a cheap unblock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants